Added a cleanup step that will mark all the memory as inaccessible and thus force termination of threads #3688
Conversation
…d thus force threads to exit
john-sharratt
force-pushed
the
force-terminate
branch
from
779bf3b
to
88eb597
Compare
| @@ -85,6 +85,9 @@ pub enum MemoryError { | |||
| /// A user defined error value, used for error cases not listed above. | |||
| #[error("A user-defined error occurred: {0}")] | |||
| Generic(String), | |||
| /// Not implemented | |||
| #[error("This operation is not yet implemented")] | |||
| NotImplemented, | |||
There was a problem hiding this comment.
This enum does not have non_exhaustive, so adding a new variant is a breaking change. We can't do this quite yet.
I suggest:
- Use the
Genericvariant - Add a TODO and create a follow up issue for the
4.0milestone to track adding a new variant - Just ignore the error in the
cleanup()code
There was a problem hiding this comment.
I understand why you would mention non_exhaustive enums but this change in the upcoming beta does not sound like a big issue to me considering all the other breaking changes that are going into that release - either way I would rather go back to the way I had it returning Ok(()) if we are not going to do it properly - this small part of the change already cost too much time.
There was a problem hiding this comment.
The problem is this is in the core Wasmer types, not in WASI.
There currently is no breaking change planned for Wasmer immediately, only later down the line.
So the next release will be 3.2, a non-breaking change.
| /// to both reads and writes. | ||
| /// `start` and `len` must be native page-size multiples and describe a range within | ||
| /// `self`'s reserved memory. | ||
| pub fn make_inaccessible(&self, start: usize, len: usize) -> Result<(), String> { |
There was a problem hiding this comment.
We should probably keep the additional API surface we expose minimal, so we can change/remove it again without a breaking change.
Wouldn't exposing make_inaccessible(&self) that equals make_all_inaccessible() be enough?
There was a problem hiding this comment.
yeah that's true - i can make this change and hide some of the API internals
There was a problem hiding this comment.
Also, really need a test or a few tests to validate the behvaiour , to make sure make_inaccessible() actually works.
Probably most easily done by starting an instance with some code that sleeps and reads some memory in a loop, then make the memory inaccessible and wait for the instance to fail?
Of course I did this I just didn't make a unit test for it - the process terminates properly when memory is made inaccessible |
theduke
force-pushed
the
force-terminate
branch
from
76d693a
to
a8f5a2b
Compare
|
I updated to master. But, @john-sharratt , I just realized: to actually use this, we need a Which we won't have available when triggering a forced shutdown externally, because the store is owned by the code calling into the instance and can't be shared. |
This pull request adds the first steps for a forced termination of processes by making all the memory that a WASM process uses inaccessible (including the stack it uses) which should terminate most threads forcefully